Use PyModule_New in module_ public constructor #2551

YannickJadoul · 2020-10-05T19:27:37Z

Alternative to the plan in #2534 and #2413: rather than making the py::module constructor private, we can turn it into a "normal" py::object constructor, and use PyModule_New to create a new module object (in the same way as types.ModuleType("abc", "def") would do so).

Modules create with PyModule_New cannot be used as extension modules (unfortunately). Python (3) complains if a module object is returned that doesn't have a def, so this is very unfortunately not a solution for the static PyModuleDef :-(

Things to note/judge/discuss:

The old PYBIND11_PLUGIN example will nót work anymore (because the py::module_ constructor has changed). This would also be the case if the constructor would be made private (though things would then just stop compiling). But, things don't segfault, and PYBIND11_PLUGIN has been deprecated for ages.
For all other uses, except for PYBIND11_PLUGIN, things should work exactly the same. PYBIND11_MODULE now uses pybind11::module_::create_extension_module, so anything that's returned as top-level extension module is correctly constructed. For all other uses, the module_ constructor should work.
I've gotten rid of detail::create_top_level_module and just moved it to a public static module_::create_extension_module. Downside: Python 2 and 3 still differ in signature, but maybe I'm getting over that. Could be reverted, as well.
One test uses the py::module_ constructor. That's why I started thinking about keeping this constructor. That test still works, as the new constructor still creates a valid module object.

I'm adding this to v2.6.0, as a way of making sure we come up with a plan on what to include now and what to postpone for the future. E.g., right now, we could judge to just deprecate the public module_ constructors, have users be warned in 2.6.0, and add this change in 2.7.0?

Suggested changelog entry:

The ``py::module_`` constructor now constructs a module object that cannot be used as top-level C extension module, after the old version was already deprecated and replaced by ``py::module_::create_extension_module``. See :ref:`the upgrade guide <upgrade-guide-2.7>` for more details.

henryiii · 2020-10-05T20:20:07Z

FWIW, I think we should probably drop PYBIND11_PLUGIN in 3.0? I've still seen code with PYBIND11_PLUGIN in it. GitHub shows over 9K results for PYBIND11_PLUGIN (many are copies of readmes, old tutorials, talks, etc. But still, probably should be a 3.0 thing, IMO)

YannickJadoul · 2020-10-05T20:27:21Z

FWIW, I think we should probably drop PYBIND11_PLUGIN in 3.0?

Right. So in that case, we cannot add this PR without breaking existing usage of PYBIND11_PLUGIN?

rwgk · 2020-10-06T17:36:10Z

To be totally honest, without having a full appreciation of all the details in this PR, I feel like this is getting way too complicated for very little gain.
I tend to think futuristic, in this case: there won't be Python 2 anymore.
Then we're left with one constructor that we don't really want to expose. From the global testing we know that (most likely) nobody ever wanted to use it anyway. Simply adding a comment: "please don't use this directly, only use our macros", would seem sufficient for all practical purposes.
More generally, to me the private feature has essential practical value for guarding against accidents that are likely to eat a lot of human time debugging, but trying to achieve purity when there isn't much of a potential for accidents anyway is unlikely to have positive value, measured in terms of human time won.

YannickJadoul · 2020-10-07T23:50:30Z

@rwgk Thanks for the review!

I'm not sure I fully understand what you mean, but let me try to answer, anyway:

This PR's diff looks horrible, but actually isn't that huge or complex, I believe. The main thing this PR tries to make sure we can treat module_ like any other py::object subclass.

But yes, I think this PR is too much to still include in 2.6.0. I've created #2552, which basically just deprecates the public constructor. As far as I'm concerned, this is basically just a formalization of your "please don't use this directly, only use our macros" comment, no? (One that's still ignorable, but less likely to be accidentally overlooked; after all, who reads the docs, let alone the comments?)

Would you mind giving #2552 a look? I've summarized the actual changes (vs. the perceived amount of diff): #2552 (comment)

I tend to think futuristic, in this case: there won't be Python 2 anymore.

As far as I know, Python 2 doesn't make things a lot more complex, in this case. But yes, completely agree that making changes becáuse of Python 2 is not a good idea anymore!

YannickJadoul · 2020-10-18T21:00:28Z

include/pybind11/pybind11.h

    explicit module_(const char *name, const char *doc = nullptr) {
-#if PY_MAJOR_VERSION >= 3
-        *this = create_extension_module(name, doc, new PyModuleDef());
+#if defined(PYPY_VERSION)


Sumitted https://foss.heptapod.net/pypy/pypy/-/merge_requests/769 to fix this

Which got merged. So once we bump PyPy, this can be fixed.

Should we wait with this PR until we bump PyPy?

I don't think we should. This is really just a wrong signature (see PyPy PR) and PyPy isn't doing anything non-const, potentially dangerous with the const_casted string. We also don't know when PyPy will release 7.4.0, and whether we'll immediately drop support for 7.3.x, then. Meanwhile, the core of this PR is good.

I did add a more specific (PYPY_VERSION_NUM < 0x07030400) check, though. This should help when we do bump the minimum PyPy version supported.

…uctor

rwgk

Google-global testing came back green for this PR, too.
Approving, but please see my other comment.

include/pybind11/pybind11.h

bstaletic

Two minor comments, but approved.

include/pybind11/pybind11.h

bstaletic · 2021-02-01T08:06:22Z

include/pybind11/pybind11.h

    explicit module_(const char *name, const char *doc = nullptr) {
-#if PY_MAJOR_VERSION >= 3
-        *this = create_extension_module(name, doc, new PyModuleDef());
+#if defined(PYPY_VERSION)


Should we wait with this PR until we bump PyPy?

YannickJadoul · 2021-02-01T21:42:59Z

@henryiii, since you haven been involved in the discussion too much: if you have time for a quick read of the upgrade guide and consider that to be clear enough, I think we're good :-)

… upgrade guide

YannickJadoul added this to the v2.6.0 milestone Oct 5, 2020

YannickJadoul requested review from henryiii, rwgk and bstaletic and removed request for henryiii October 5, 2020 19:27

YannickJadoul mentioned this pull request Oct 5, 2020

Deprecate public constructors of module_ class #2552

Merged

EricCousineau-TRI mentioned this pull request Oct 6, 2020

common: Prevent module re-import duplication in Python3 #1558

Closed

YannickJadoul removed this from the v2.6.0 milestone Oct 7, 2020

YannickJadoul force-pushed the module-public-ctr branch 4 times, most recently from b043c6d to 65f8c98 Compare October 8, 2020 16:14

YannickJadoul force-pushed the module-public-ctr branch from 65f8c98 to b479f2a Compare October 18, 2020 19:21

YannickJadoul commented Oct 18, 2020

View reviewed changes

YannickJadoul mentioned this pull request Dec 8, 2020

[QUESTION] De-deprecation of direct call to module constructor #2718

Open

Use PyModule_New in module_ public constructor

2436f44

YannickJadoul force-pushed the module-public-ctr branch from b479f2a to 2436f44 Compare December 8, 2020 17:15

YannickJadoul mentioned this pull request Dec 15, 2020

Use default arguments in create_extension_module #2717

Closed

YannickJadoul added this to the v2.7 milestone Dec 17, 2020

YannickJadoul closed this Jan 30, 2021

YannickJadoul reopened this Jan 30, 2021

Cleanup leak in test_modules by using new non-extension-module constr…

1d0660f

…uctor

YannickJadoul force-pushed the module-public-ctr branch from c8025b1 to 1d0660f Compare January 30, 2021 22:12

rwgk approved these changes Feb 1, 2021

View reviewed changes

include/pybind11/pybind11.h Show resolved Hide resolved

bstaletic approved these changes Feb 1, 2021

View reviewed changes

YannickJadoul added 2 commits February 1, 2021 21:45

Add version check to const_cast for PyPy's PyModule_New

01a53c7

Fix allocation failure error message

7f64ed3

YannickJadoul force-pushed the module-public-ctr branch from 126da01 to 10f8790 Compare February 1, 2021 21:39

Document the new module_ constructor and add an extensive note to the…

29675ed

… upgrade guide

YannickJadoul force-pushed the module-public-ctr branch from 10f8790 to 29675ed Compare February 1, 2021 21:58

henryiii modified the milestones: v2.7, v2.8 Jul 16, 2021

henryiii modified the milestones: v2.8, v2.9 Oct 4, 2021

rwgk mentioned this pull request Feb 10, 2023

FWD pybind11 google/pybind11clif#2551

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use PyModule_New in module_ public constructor #2551

Use PyModule_New in module_ public constructor #2551

YannickJadoul commented Oct 5, 2020 •

edited

Loading

henryiii commented Oct 5, 2020

YannickJadoul commented Oct 5, 2020

rwgk commented Oct 6, 2020

YannickJadoul commented Oct 7, 2020

YannickJadoul Oct 18, 2020

YannickJadoul Jan 30, 2021

bstaletic Feb 1, 2021

YannickJadoul Feb 1, 2021

rwgk left a comment

bstaletic left a comment

bstaletic Feb 1, 2021

YannickJadoul commented Feb 1, 2021

Use PyModule_New in module_ public constructor #2551

Are you sure you want to change the base?

Use PyModule_New in module_ public constructor #2551

Conversation

YannickJadoul commented Oct 5, 2020 • edited Loading

Suggested changelog entry:

henryiii commented Oct 5, 2020

YannickJadoul commented Oct 5, 2020

rwgk commented Oct 6, 2020

YannickJadoul commented Oct 7, 2020

YannickJadoul Oct 18, 2020

Choose a reason for hiding this comment

YannickJadoul Jan 30, 2021

Choose a reason for hiding this comment

bstaletic Feb 1, 2021

Choose a reason for hiding this comment

YannickJadoul Feb 1, 2021

Choose a reason for hiding this comment

rwgk left a comment

Choose a reason for hiding this comment

bstaletic left a comment

Choose a reason for hiding this comment

bstaletic Feb 1, 2021

Choose a reason for hiding this comment

YannickJadoul commented Feb 1, 2021

YannickJadoul commented Oct 5, 2020 •

edited

Loading